Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc_lint: Remove unused_crate_dependencies from the unused group #72702

Merged
merged 1 commit into from
May 29, 2020

Conversation

petrochenkov
Copy link
Contributor

Fixes #72686

It's undesirable to enable unused_crate_dependencies with blanket #![deny(unused)] due to the amount of redundant --extern options passed by Cargo.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

Hm, does the lint itself have lots of false positives then? Most projects deny all warnings in CI, so whether it's part of unused or not doesn't seem like it would matter that much?

@petrochenkov
Copy link
Contributor Author

@Mark-Simulacrum
#![deny(warnings)] doesn't deny allow-by-default lints, only warn-by-default ones.
For example, https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=9bd06857be49d26a979aece1b997e71b

@estebank
Copy link
Contributor

r=me if you wish. Can we create a tracking ticket to make sure we eventually figure out how to make cargo not trigger this unecessarily?

@petrochenkov
Copy link
Contributor Author

@bors r=estebank rollup

Can we create a tracking ticket to make sure we eventually figure out how to make cargo not trigger this unecessarily?

I'm not sure whether it's a part of rust-lang/cargo#7916 or some other related existing issue.
cc @ehuss

@bors
Copy link
Contributor

bors commented May 28, 2020

📌 Commit 1eef0c3 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2020
@Mark-Simulacrum
Copy link
Member

Oh I think I missed that it's already allow-by-default then, thanks!

@ehuss
Copy link
Contributor

ehuss commented May 28, 2020

I don't think it is possible for Cargo to support this lint as-is for normal builds (without the false positives).

There are a few alternatives:

  • Provide cargo-target-specific dependencies (such as Allow specifying dependencies for individual artifacts rfcs#2887). Unfortunately this would require the user to very carefully separate the dependency declarations, and I suspect that will be awkward and difficult in some situations.
  • Use cargo-udeps which I think is the only way to solve this right now for Cargo projects.

As I mentioned in the original PR, I feel like this lint is only useful for non-cargo build systems, or adventurous people who want to wade through the false positives. I would recommend cargo-udeps otherwise. It might be possible for cargo-udeps to use this lint as an alternative to its other approaches (it has multiple backends).

@Mark-Simulacrum
Copy link
Member

One workflow that I might imagine where we could reduce the false-positives is to tell Cargo in some JSON-y format about the diagnostics, but not actually emit them by default. Then Cargo would aggregate them and be able to say "this dependency is not used by any target" -- in particular, to detect cases where e.g. you stopped using a dependency entirely.

I agree that this happens somewhat rarely so maybe is not the most important thing to do, though.

@est31
Copy link
Member

est31 commented May 28, 2020

@Mark-Simulacrum see discussion in #57274. You can bikeshed a lot about the design whether to make rustc emit the used dependencies or the unused ones, and which format to use (modifying the existing stdout json format, storing json to disk, stabilizing build-dep-depinfo and using .dep files, etc), but:

  • any form of the lint that's not full of false positives needs coordination at the Cargo level.

  • The rustc lint of Warn about unused crate deps #72342 is useful but for build.rs and non-cargo users (like the PR author) and people for whom it's easier to sift through the false positives than using cargo-udeps.

Maybe I should make a table about which cargo subcommand has enough data to check which dependency section in Cargo.toml.

bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#72239 (Implement PartialOrd and Ord for SocketAddr*)
 - rust-lang#72466 (Stabilize str_strip feature)
 - rust-lang#72605 (Add working example for E0617 explanation)
 - rust-lang#72636 (Cleanup `Resolver::<clone|into>_outputs` methods)
 - rust-lang#72645 (Add myself to .mailmap)
 - rust-lang#72667 (expand unaligned_references test)
 - rust-lang#72670 (Fix incorrect comment in generator test)
 - rust-lang#72674 (Clippy should always build)
 - rust-lang#72682 (Add test for rust-lang#66930)
 - rust-lang#72695 (update data layout for illumos x86)
 - rust-lang#72697 (Remove rustc-ux-guidelines)
 - rust-lang#72702 (rustc_lint: Remove `unused_crate_dependencies` from the `unused` group)

Failed merges:

r? @ghost
@bors bors merged commit 049b6dd into rust-lang:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positives for dev-dependencies with unused_crate_dependencies lint
8 participants